feat(account): add possibility to block #73
Conversation
Table added for blocking accounts.
Table policies and functions adjusted to exclude data of blocked accounts.
There was a problem hiding this comment.
Good work, the EXCEPT keyword is actually new to me!
In general, these changes definitely make tests necessary now, I'd say, as the complexity of all conditions grew to a point now where the overall meaning is not easily understandable at a glance anymore.
Would you fill some files in the verify folder please by adding some test INSERTs that mimic the most crucial use cases?
"A blocked B, so B's contact is not visible for A any more" as a simple example.
| -- requires: role_account | ||
| -- requires: role_anonymous | ||
|
|
||
| BEGIN; | ||
|
|
||
| CREATE FUNCTION maevsi_private.events_invited() | ||
| RETURNS TABLE (event_id UUID) AS $$ | ||
| CREATE OR REPLACE FUNCTION maevsi_private.events_invited() |
There was a problem hiding this comment.
Why OR REPLACE? The way migrations are applied or reverted currently does not allow for any case where this function could be replaced, I think.
| SELECT id | ||
| FROM maevsi.contact | ||
| WHERE | ||
| jwt_account_id IS NOT NULL |
There was a problem hiding this comment.
Why is the IS NOT NULL check removed here? I think it's likely necessary to be kept as I once found it necessary to add the comment regarding it below.
The contact selection does not return rows where account_id "IS" null due to the equality comparison.
| $$ LANGUAGE plpgsql STABLE STRICT SECURITY DEFINER | ||
| ; |
There was a problem hiding this comment.
| $$ LANGUAGE plpgsql STABLE STRICT SECURITY DEFINER | |
| ; | |
| $$ LANGUAGE plpgsql STABLE STRICT SECURITY DEFINER; |
Let's keep the formatting as is and change it only in a separate PR if wanted.
| author_account_id NOT IN ( | ||
| SELECT account_block_id | ||
| FROM maevsi.account_block | ||
| WHERE b.author_account_id = jwt_account_id |
| DECLARE | ||
| jwt_account_id UUID; | ||
| BEGIN | ||
| jwt_account_id := NULLIF(current_setting('jwt.claims.account_id', true), '')::UUID; | ||
|
|
||
| RETURN QUERY | ||
| SELECT invitation.event_id FROM maevsi.invitation | ||
| SELECT event_id FROM maevsi.invitation |
There was a problem hiding this comment.
In a few places it's necessary to keep this prefix or the column name would be ambiguous, but maybe this is not necessary here any more if you've checked it and it works without. If this function is confirmed to still be working, you can resolve this comment.
| invitee_count_maximum IS NULL | ||
| OR | ||
| invitee_count_maximum > (maevsi.invitee_count(id)) -- Using the function here is required as there would otherwise be infinite recursion. | ||
| ) |
There was a problem hiding this comment.
Just fyi: changing the indentation while also having changes to the code makes it hard to read the difference for reviewers as you can see in above. The code stays mostly the same here, but there are way more green and red lines than necessary for an addition. Therefore, it's better to change formatting in a separate PR. You can resolve this comment once understood.
| -- Only allow inserts for invitations issued to a contact that was created by oneself | ||
| -- Do not allow inserts for invitations issued to a contact referring a blocked account |
There was a problem hiding this comment.
| -- Only allow inserts for invitations issued to a contact that was created by oneself | |
| -- Do not allow inserts for invitations issued to a contact referring a blocked account | |
| -- Only allow inserts for invitations issued to a contact that was created by oneself. | |
| -- Do not allow inserts for invitations issued to a contact referring a blocked account. |
| -- Only allow updates to invitations issued to oneself through the account, but not invitations auhored by a blocked account | ||
| -- Only allow updates to invitations to events organized by oneself, but not invitations issued to a blocked account or issued by a blocked account |
There was a problem hiding this comment.
| -- Only allow updates to invitations issued to oneself through the account, but not invitations auhored by a blocked account | |
| -- Only allow updates to invitations to events organized by oneself, but not invitations issued to a blocked account or issued by a blocked account | |
| -- Only allow updates to invitations issued to oneself through the account, but not invitations auhored by a blocked account. | |
| -- Only allow updates to invitations to events organized by oneself, but not invitations issued to a blocked account or issued by a blocked account. |
| NULLIF(current_setting('jwt.claims.account_id', true), '')::UUID IS NOT NULL | ||
| AND |
There was a problem hiding this comment.
I think the same as outlined above applies here: anonymous users could update invitations for contacts that have no account assigned.
| table_account_block [schema_public table_account_public] 1970-01-01T00:00:00Z Sven Thelemann <sven.thelemann@t-online.de> # Blocking of an account by another account. | ||
| table_account_block_policy [schema_public table_account_block role_account] 1970-01-01T00:00:00Z Sven Thelemann <sven.thelemann@t-online.de> # Policy for table account block. |
There was a problem hiding this comment.
These new elements should be referenced by the other elements below that make use of them. It's even more important to have them used in the lines below in this file than to have them mentioned as comments in the actual source files.
The comments in the source files are just a helper for our brains, but the state defined in this plan file here is what's actually verified.
Table added for blocking accounts.